Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Proxy / CDN Config Implementation #136

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

kirkchris
Copy link

Regarding #92

Happy to make changes based on feedback.

@kirkchris kirkchris mentioned this pull request Feb 7, 2018
@@ -29,6 +29,7 @@ const defaultOptions = {
puppeteerExecutablePath: undefined,
puppeteerIgnoreHTTPSErrors: false,
publicPath: "/",
proxy: {},
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is proxy config in CRA which I plan eventually support, so there is a name clash. Not sure what to do about it. Otherwise PR looks fine, but haven't tested it myself

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verified in our own deploy and code works. We're using it in production now.

Any thoughts on name? I can use cdn if you think that's better?

Copy link
Owner

@stereobooster stereobooster Feb 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Three options (which I do not like)

  • proxy (good, but clashes with other one)
  • browserProxy
  • cdn
  • thirdPartyResources

There are only two hard problems in the programming...

Copy link
Author

@kirkchris kirkchris Mar 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bah, sorry about the delay on this... I didn't have any brilliant ideas on the naming 😒 I think of those, I probably like thirdPartyResources the best. My concern with cdn is that it wouldn't always be a cdn, just any third party. What do you think?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are not delaying it. It is rather me, who have not much time recently to do updates on this project. I didn't expect that you alone should come up with the name. I still have no good idea either

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pardon my stupidity, but how does this clash? i use CRA and reactSnap's config is a separate object, so reactSnap.proxy doesn't conflict with proxy with CRA uses. though if it does, what do you think of snapProxy for a name?

Copy link

@jayenashar jayenashar May 22, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

after trying to write tests for this in #179, this seems very different to CRA's proxy. this would be better called cdn to avoid confusion.

@stereobooster
Copy link
Owner

At the moment I'm trying to write proper tests for the package. It was without tests way too long. This means, that all other PRs will be on hold. See #171

@atomic-tang
Copy link

atomic-tang commented Aug 31, 2018

I am currently testing out react-snap with my current project where I had setup an api proxy, as per the CRA documentation, to handle API calls to the backend. When the react-snap script runs after building the project, I checked the response for one of my API calls and I was getting the html markup of my index.html file instead of the expected JSON body. Just wanted to know if I've missed something as I have looked around and could not find anything to solve my issue. I've tried replacing one of my fetch API urls with the absolute path of the API end point and I am now getting the correct response. But I don't want to do it this way for security reasons.

@Misiu
Copy link

Misiu commented Oct 9, 2019

@stereobooster any chance this might get added?
I'm working on a project that needs all the static resources (files from build\static) server from a separate domain.
YSlow is also suggesting this:

Use cookie-free domains
Serve static content from a different domain to avoid unnecessary cookie traffic.

React-snap is really great, but this feature is missing. I'm aware it wasn't requested a lot, but the issue related to this is from 2 Jan 2018.

@bwiklund
Copy link

bwiklund commented May 6, 2020

any news on this? i haven't found any way to get a proxy working during the site build and it's a big pain point

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants